@claude review

Claude finished @BobDickinson's task in 3m 21s —— View job

Code Review — PR #1609 (migrate web OAuth to RemoteOAuthStorage)

• Read the PR diff against origin/v2/main
• Review new remoteOAuthStorage.ts + tests
• Review environmentFactory.ts, App.tsx, clearServerOAuthState.ts changes
• Check RemoteOAuthStorage core adapter semantics
• Post review feedback

Nice, well-scoped refactor — the plumbing of oauthStorage into clearServerOAuthState and the typed test stubs (dropping as never) are clean improvements, and keeping core/ untouched is the right call. My main feedback is one architectural risk worth verifying before merge.

🔴 Main concern: sync reads/writes now race async hydration

The old BrowserOAuthStorage was backed by sessionStorage, whose getItem is synchronous — so Zustand's persist middleware hydrated the store synchronously during createStore. Every subsequent read (getCodeVerifier, getClientInformation, …) was guaranteed to see the persisted state.

RemoteOAuthStorage is backed by an async adapter (core/storage/adapters/remote-storage.ts — getItem is async + HTTP GET). Zustand hydrates such stores asynchronously in the background; createOAuthStore (core/auth/store.ts) has no hydration barrier. But the reads in OAuthStorageBase are still synchronous against in-memory state:

// core/auth/providers.ts:242
codeVerifier(): string {
  const verifier = this.storage.getCodeVerifier(this.serverUrl); // reads get().servers[url] in memory
  if (!verifier) throw new Error("No code verifier saved for session");
  return verifier;
}

Two failure modes fall out of this:

1. Callback code-exchange can throw. On the /oauth/callback fresh page load, webOAuthStorage is created at App mount and the effect proceeds as soon as servers.length > 0 (App.tsx:2118). That gate is a separate backend fetch — nothing awaits the /api/storage/oauth hydration GET. If servers hydrate first, resumeAfterOAuth → codeVerifier() reads an empty store and throws "No code verifier saved for session", dropping the user into the re-auth banner. This is precisely the scenario the PR aims to make more robust, so it'd be good to prove it can't regress.

2. Writes before hydration can clobber oauth.json. setServerState/clearServerState call Zustand set(), which persists the full in-memory servers map. If a write (e.g. saveCodeVerifier on connect, or clear() from "clear stored OAuth") lands before hydration merges the on-disk blob, the persisted state is { servers: { [thisUrl]: … } } — silently wiping every other server's tokens that existed on disk. With tab-local sessionStorage this couldn't happen (sync hydration + per-tab scope); with a shared file it can.

Both are timing-dependent, so they may pass in a quick manual test and bite intermittently. Worth confirming there's a hydration await on these paths, e.g. gate the callback effect (and ideally the first connect/clear) on persist's onFinishHydration / hasHydrated, or expose a whenHydrated() on the storage that resumeAfterOAuth awaits. Investigate this →

🟡 Minor

• getRemoteOAuthStorage is a single-slot cache, not a per-key map (remoteOAuthStorage.ts:7). The doc comment and PR body say "memoizes one instance per { origin, authToken }", but cached holds only the most recent key — alternating two keys would thrash and hand out fresh Zustand stores (divergent in-memory state). In practice only one {origin, token} is live per page, so this is harmless today; consider a Map if you want the comment to hold literally, or tighten the wording to "memoizes the last requested key."
• resetWebRemoteOAuthStorageCacheForTests ships in the production bundle. It's @internal-tagged so this is acceptable, just flagging it's reachable at runtime.
• Cross-tab sharing is load-time only. Because persist hydrates once at store creation, a tab reads oauth.json only on load — live cross-tab propagation isn't there (a second tab won't see a first tab's new token until it reloads). That matches the "survive refresh" framing and the navigator.locks follow-up note, so no change needed; just making sure the "shared with CLI/TUI" claim is understood as load-time consistency.

✅ Looks good

• clearServerOAuthState now takes oauthStorage + narrows inspectorClient to Pick<…, "clearOAuthTokens"> — good testability and least-privilege typing.
• useMemo([], getAuthToken()) is consistent with the existing token-is-stable-for-page-lifetime assumption (getAuthToken is module-level at App.tsx:214).
• Test coverage additions are solid and the vite.config.ts src/lib/** include keeps the gate honest.
  • v2/1548-web-remote-oauth-storage

PR review response — async OAuth storage / callback race

Re: review feedback on RemoteOAuthStorage + async persistence

The latest commit (a2409148 — fix(auth): await OAuth storage load() to fix remote OAuth races) addresses the main concern about RemoteOAuthStorage + Zustand persist loading over HTTP while callers still treated storage as immediately ready.

1. Callback reads PKCE before persisted state is loaded

/oauth/callback now calls await webOAuthStorage.load() before resumeAfterOAuth, so the code exchange does not read getCodeVerifier() against an empty in-memory store while the GET to /api/storage/oauth is still in flight.

2. Writes can clobber oauth.json with an empty snapshot

OAuthStorage now has an explicit load(): Promise<void>. OAuthStorageBase implements it (waits until the backing adapter has read persisted state). All mutating methods — save*, clear*, etc. — call ensureLoaded() first, so we do not persist before the initial load completes.

3. Code exchange path

completeOAuthFlow in oauthManager also awaits storage.load() before reading the verifier, so the same race is covered outside the web callback path (CLI/TUI/EMA).

4. API shape

Public name is load(), not Zustand “hydration” terminology. Sync getters (getCodeVerifier, getScope, …) are unchanged; callers that depend on persisted values must await load() first (or use the async mutators, which load internally). clearOAuthTokens / related clears are async end-to-end where needed (e.g. TUI await clearOAuthTokens()).

5. Tests

• storage-remote.test.ts: regression test that load() waits on a delayed GET before sync reads see persisted state.
• Storage/integration tests updated to await async saves/clears and to mock load on stubs.

Out of scope for this commit (as discussed)

• Removing Zustand (Remove Zustand file-store wrappers; auto-convert to plain JSON on read #1549) — still deferred.
• Full root npm run coverage for mid-session CLI/TUI gaps — follow-up PR.

@claude review

Claude finished @BobDickinson's task in 2m 57s —— View job

Re-review after the load() fix (a2409148)

• Read the full PR diff against origin/v2/main
• Verify the load() hydration barrier addresses the earlier race concerns
• Review new/changed core auth storage code
• Post review

The load() barrier is the right shape and resolves both earlier race concerns:

• Callback code-exchange — App.tsx:2194 now await webOAuthStorage.load() before resumeAfterOAuth, and oauthManager.completeOAuthFlow (oauthManager.ts:285) awaits requireStorage().load() before reading the verifier. So the sync getCodeVerifier() in providers.ts:243 can no longer read an empty store. ✅
• Write-before-hydration clobber — every mutator (save*/clear*) now await ensureLoaded() before set(), so the on-disk servers map is merged in before any partial write persists. ✅
• The sync getters that stay unguarded (getScope/getServerMetadata/getClientRegistrationKind in connection-state.ts:90-97) are fine because computeConnectionState awaits getTokens() first (which calls ensureLoaded), so hydration is done before the sync reads. Good.

Nicely done — the async plumbing through clearOAuthTokens/clearEmaIdpSession/clearServerOAuthState and the TUI await is consistent.

🔴 One new failure mode introduced by load(): a failed hydration hangs forever

OAuthStorageBase.load() (core/auth/oauth-storage.ts:39-50) resolves only via persist.hasHydrated() or persist.onFinishHydration():

load(): Promise<void> {
  if (!this.loadedPromise) {
    this.loadedPromise = new Promise((resolve) => {
      if (this.store.persist.hasHydrated()) { resolve(); return; }
      this.store.persist.onFinishHydration(() => resolve());
    });
  }
  return this.loadedPromise;
}

In Zustand v5's persist middleware, if the storage adapter's getItem rejects, the hydrate chain lands in .catch, which calls onRehydrateStorage(undefined, e) but does not set hasHydrated = true and does not fire the finishHydration listeners. So on a rejected hydration, hasHydrated() stays false forever and onFinishHydration never fires → load() never settles.

The remote adapter rejects on any non-OK, non-404 response (remote-storage.ts:41-45: throw new Error("Failed to read store: " + status)) — e.g. a 500 from the backend, a 401 if the token is rejected, or a network-level fetch rejection. When that happens:

• await webOAuthStorage.load() in the callback effect (App.tsx:2194) hangs — the try/catch around resumeAfterOAuth never runs, so no re-auth banner, no error, the flow just stalls silently.
• completeOAuthFlow (oauthManager.ts:285) hangs on the same await.
• Every mutator (ensureLoaded) hangs too, so connect / clear-OAuth stall with no feedback.

With the old sessionStorage backing this was impossible (sync getItem, never rejects). Moving to an HTTP-backed adapter makes it reachable, and the symptom (a permanent silent hang) is arguably worse than the pre-fix throw, which at least surfaced the banner.

Suggested fix: settle load() on hydration error as well as success. The clean route is to capture completion in createOAuthStore via the onRehydrateStorage option (it receives (state, error) on both paths) and expose that to OAuthStorageBase, so load() resolves (or rejects, if you'd rather surface the banner) when hydration fails. A minimal alternative is to add onRehydrateStorage: () => (_state, _error) => {/* mark loaded */} and have load() await that signal instead of relying solely on onFinishHydration.

Fix this →

🟡 Minor (carried over / still applicable)

• Add a test for the failed-hydration path. storage-remote.test.ts now covers the delayed GET (load waits, then sync reads see state), which is great — but not the rejected GET. A test asserting load() settles (however you decide it should behave) when the GET returns 500 would lock in the fix above.
• getRemoteOAuthStorage is still a single-slot cache (remoteOAuthStorage.ts:8), not a per-key Map as the doc comment implies. Harmless today (one {origin, token} per page); either switch to a Map or soften the comment to "last requested key."

✅ Looks good

• load() memoization via loadedPromise (single hydration wait, idempotent) is clean.
• The interface change to OAuthStorage (adding load(), making clears Promise) is coherent and the call sites were updated consistently across web/TUI/core/EMA.
• The computeConnectionState sync-getter ordering (async getTokens first) is correct — good that hydration is forced before the sync reads.
  • branch v2/1548-web-remote-oauth-storage

PR comment — settle load() on persist read failure

Addresses Claude re-review (post-a2409148): load() hung forever when remote GET rejected, because Zustand persist only fires onFinishHydration on success, not when the storage adapter throws.

Fix

• createOAuthStore: onRehydrateStorage settles a per-store promise on both success and error; load() rejects when hydration fails (no silent hang, no mutating against an unresolved read).
• OAuthStorageBase.load(): awaits that signal instead of relying solely on onFinishHydration.
• App.tsx OAuth callback: try/catch around await webOAuthStorage.load() so a failed GET surfaces the re-auth banner instead of stalling the effect.
• Test: load() rejects when remote GET fails instead of hanging in storage-remote.test.ts.

Note on store.ts shape

The WeakMap / settle promise wiring is Zustand persist glue — necessary until #1549 replaces persist with explicit blob load/save. Not the long-term design.

Validation

• npm run ci — green
• clients/web npm run test:coverage — green

@claude review

Claude finished @BobDickinson's task in 2m 6s —— View job

Re-review after 512f45d3 (reject load() on persist read failure)

• Read the latest diff vs origin/v2/main
• Verify the failed-hydration hang fix (load() now rejects)
• Check the callback / mutator error paths surface correctly
• Re-check carried-over minor items

The failed-hydration concern from the last round is resolved correctly. createOAuthStore now wires onRehydrateStorage: () => (_state, error) => settlePersistLoad(error), settlePersistLoad rejects a per-store promise on error, and load() awaits it via waitForOAuthStorePersistLoad. So a 500/401/network failure on the /api/storage/oauth GET now makes load() reject instead of hanging:

• Callback (App.tsx:2194) — try { await webOAuthStorage.load() } catch clears connectStartRef and shows the re-auth banner. ✅ (Good catch on resetting the ref guard so a retry isn't blocked.)
• Code exchange (oauthManager.ts:285) — await this.requireStorage().load() is inside the existing try in completeOAuthFlow, so a rejection routes to the normal error handling. ✅
• Mutators — ensureLoaded() now rejects rather than hanging; connect/clear surface an error instead of stalling silently. ✅
• Regression test load() rejects when remote GET fails instead of hanging (storage-remote.test.ts) locks in the 500 path. ✅

Nice work — the settlePersistLoad idempotency guard and the hasHydrated() short-circuit in load() (so the success path never awaits a second time) are both correct.

🟡 One thing worth considering: eager reject → possible unhandled rejection

persistLoadPromise is created eagerly in createOAuthStore and stored in the WeakMap, but the only thing that ever attaches a handler to it is a call to load()/waitForOAuthStorePersistLoad. If the hydration GET rejects before any consumer calls load(), that promise rejects with no .catch attached → an unhandled promise rejection (console warning in the browser; in Node/tests it can trip unhandledRejection).

In the main web flow this is usually masked: computeConnectionState → getTokens() → ensureLoaded() attaches a handler in a microtask/effect, which runs well before an HTTP fetch can settle. But it's timing-dependent, and any store that's created and then never has load() called on it (e.g. an idle/short-lived store, or certain EMA-disabled paths) would emit the warning on a backend error.

If you want to be defensive, attach a no-op catch at creation so the rejection is always considered handled, while real awaiters still see it:

persistLoadPromise.catch(() => {}); // keep the eager promise from going "unhandled"
persistLoadByStore.set(store, persistLoadPromise);

Not blocking — just cheap insurance now that this promise can reject. Consider this →

🟡 Minor (still open from earlier)

• getRemoteOAuthStorage is still a single-slot cache (remoteOAuthStorage.ts:8), while the doc comment says "Memoized by { baseUrl, authToken }". Harmless today (one {origin, token} per page), but the comment over-promises — either use a Map or soften the wording to "the last requested key."

✅ Looks good

• The load() barrier + reject-on-error is a coherent, minimal shape for the async adapter, and the Promise-returning clear*/clearOAuthTokens/clearIdpSession conversions were plumbed through web/TUI/core/EMA consistently (spot-checked providers.clear, oauthManager.clearOAuthTokens, clearServerOAuthState).
• The computeConnectionState sync-getter ordering (async getTokens first, which forces ensureLoaded) remains correct.
• Test updates to await the now-async mutators are thorough, and the delayed-GET + failed-GET tests together cover the load-barrier behavior well.

Overall: the correctness concern is closed; the remaining note is a defensive polish, not a blocker.
• branch v2/1548-web-remote-oauth-storage

Regarding the "eager rejection" recommendation - that entire class of problem goes away with the removal of Zustand storage, which will be a fast follow. I'm happy to go with what we have now so we can get to that work.